cmd/leaderboard/leaderboard.go (1)

Line range hint 13-26: Add unit tests for the leaderboard command

The New() function lacks test coverage. Consider adding tests to verify:

• Command initialization
• Subcommand registration
• Command properties (Use, Short, Long descriptions)

Would you like me to help create a test file with comprehensive test cases for the leaderboard command initialization?

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 13-13: cmd/leaderboard/leaderboard.go#L13
Added line #L13 was not covered by tests

cmd/query/query.go (1)

12-14: Add test coverage for command metadata

The command metadata is well-documented with clear descriptions. However, these changes aren't covered by tests.

Would you like me to help create test cases to verify the command metadata? This could include:

• Verifying command name and aliases
• Validating help text output
• Testing command initialization

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 12-14: cmd/query/query.go#L12-L14
Added lines #L12 - L14 were not covered by tests

cmd/ingest/sbom/sbom.go (1)

60-61: Add test coverage for command initialization

The command initialization code lacks test coverage. Consider adding tests to verify:

• Command creation with default options
• Flag registration
• Command validation

Would you like me to help create test cases for the command initialization? This would help ensure the refactored code remains stable.

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 60-61: cmd/ingest/sbom/sbom.go#L60-L61
Added lines #L60 - L61 were not covered by tests

cmd/ingest/scorecard/scorecard.go (1)

Line range hint 1-83: Good architectural improvement in progress.

The removal of the storage dependency from the command layer is a positive architectural change that:

1. Reduces coupling between the command layer and storage implementation
2. Makes the code more maintainable and testable
3. Follows the separation of concerns principle

Consider documenting these architectural decisions in the project's documentation to help future contributors understand the design choices.

pkg/tools/ingest/loader.go (2)

Line range hint 37-44: Remove duplicate error handling block

There's a duplicate error check and append operation in the directory processing loop. The second error check is redundant as the error is already handled in the first block.

 subResult, err := LoadDataFromPath(entryPath)
 if err != nil {
     errors = append(errors, fmt.Errorf("failed to load data from path %s: %w", entryPath, err))
 } else {
     result = append(result, subResult...)
 }
-if err != nil {
-    errors = append(errors, fmt.Errorf("failed to ingest data from path %s: %w", entryPath, err))
-}

---

19-19: Update function documentation to reflect changes

The function comment should be updated to reflect the removal of the storage parameter and clearly document the function's current responsibilities:

-// LoadDataFromPath takes in a directory or file path and processes the data into the storage.
-// The data can either be JSON files or ZIP files containing JSON files.
+// LoadDataFromPath processes data from a given file path, supporting both JSON files and ZIP archives
+// containing JSON files. It recursively processes directories and returns the collected data.
+// Returns a slice of Data containing the file paths and their contents, or an error if any operation fails.

cmd/query/custom/custom.go (2)

18-25: LGTM! Good architectural improvements

The refactoring of the options struct shows good separation of concerns:

• Removal of storage dependency aligns with the service-oriented architecture
• Addition of queryServiceClient enables better testing through dependency injection
• New output options provide better flexibility for different use cases

Consider adding documentation for the output formats and valid values in the struct tags or package documentation.

---

28-34: Add validation for the output format flag

While the flag setup is good, consider adding validation for the output format during flag parsing to fail fast if an invalid format is provided.

 func (o *options) AddFlags(cmd *cobra.Command) {
 	cmd.Flags().IntVar(&o.maxOutput, "max-output", 10, "maximum number of results to display")
 	cmd.Flags().BoolVar(&o.showInfo, "show-info", true, "display the info column")
 	cmd.Flags().StringVar(&o.addr, "addr", "http://localhost:8089", "address of the minefield server")
-	cmd.Flags().StringVar(&o.output, "output", "table", "output format (table or json)")
+	cmd.Flags().StringVar(&o.output, "output", "table", "output format (table or json)")
+	cmd.RegisterFlagCompletionFunc("output", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
+		return []string{"table", "json"}, cobra.ShellCompDirectiveNoFileComp
+	})
 }

pkg/tools/ingest/vuln_test.go (1)

Line range hint 36-54: Consider making the test more resilient to test data changes

While the removal of the storage parameter is handled correctly, the test assumes a very specific outcome (exactly 3 new nodes). This might make the test brittle if the test data changes.

Consider:

1. Adding a comment explaining why exactly 3 new nodes are expected
2. Making the test more resilient by either:
   • Calculating the expected number of nodes based on the test data
   • Using a range check instead of an exact number

-	if len(keys) != numberOfNodes+3 {
-		t.Fatalf("Expected number of nodes to be %d, got %d", numberOfNodes+3, len(keys))
+	// Add comment explaining the expected node increase
+	expectedMinIncrease := 1 // minimum number of new vulnerability nodes
+	if len(keys) <= numberOfNodes || len(keys) > numberOfNodes+10 {
+		t.Fatalf("Expected number of nodes to increase by at least %d, got an increase of %d", 
+			expectedMinIncrease, len(keys)-numberOfNodes)
+	}

api/v1/service_test.go (1)

Line range hint 76-124: Consider breaking down the test into smaller, focused test cases.

The TestQueriesIngestAndCache function is testing multiple operations in a single test case, which can make it difficult to maintain and debug. Consider splitting it into separate test cases for each operation:

• TestSBOMIngestion
• TestVulnerabilityIngestion
• TestCustomLeaderboard
• TestDependencyQueries
• TestCacheOperations

This would improve:

• Test isolation
• Failure debugging
• Maintenance
• Readability

Example refactor for the SBOM ingestion test:

func TestSBOMIngestion(t *testing.T) {
    s := setupService()
    
    // Load and ingest SBOM
    result, err := ingest.LoadDataFromPath("../../testdata/osv-sboms/google_agi.sbom.json")
    require.NoError(t, err)
    
    for _, data := range result {
        sbomReq := connect.NewRequest(&service.IngestSBOMRequest{
            Sbom: data.Data,
        })
        _, err = s.IngestSBOM(context.Background(), sbomReq)
        require.NoError(t, err)
    }
    
    // Verify ingestion
    graphReq := connect.NewRequest(&service.GetNodeByNameRequest{
        Name: "pkg:pypi/astroid@2.11.7",
    })
    resp, err := s.GetNodeByName(context.Background(), graphReq)
    require.NoError(t, err)
    assert.NotNil(t, resp.Msg.Node)
}

cmd/query/custom/custom_test.go (2)

17-62: Consider refactoring to use table-driven tests

The test could be more maintainable and concise using a table-driven approach with the assertion library.

Here's a suggested refactoring:

 func TestOptions_AddFlags(t *testing.T) {
 	o := &options{}
 	cmd := &cobra.Command{}
 	o.AddFlags(cmd)
-
-	// Test "max-output" flag
-	maxOutputFlag := cmd.Flags().Lookup("max-output")
-	if maxOutputFlag == nil {
-		t.Error("Expected 'max-output' flag to be defined")
-	} else {
-		if maxOutputFlag.DefValue != "10" {
-			t.Errorf("Expected default value of 'max-output' to be '10', got '%s'", maxOutputFlag.DefValue)
-		}
-	}
-	// ... similar blocks for other flags
+
+	tests := []struct {
+		flagName    string
+		defValue    string
+	}{
+		{"max-output", "10"},
+		{"show-info", "true"},
+		{"addr", "http://localhost:8089"},
+		{"output", "table"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.flagName, func(t *testing.T) {
+			flag := cmd.Flags().Lookup(tt.flagName)
+			assert.NotNil(t, flag, "Expected '%s' flag to be defined", tt.flagName)
+			assert.Equal(t, tt.defValue, flag.DefValue, 
+				"Expected default value of '%s' to be '%s'", tt.flagName, tt.defValue)
+		})
+	}

---

64-115: Consider refactoring command property checks

Similar to the previous suggestion, the command property checks could be more maintainable using table-driven tests.

Here's a suggested refactoring for the command properties section:

 func TestNewCommand(t *testing.T) {
 	cmd := New()
-	if cmd == nil {
-		t.Fatal("Expected New() to return a non-nil command")
-	}
+	assert.NotNil(t, cmd, "Expected New() to return a non-nil command")
 
-	// Check the command's basic properties
-	if cmd.Use != "custom [script]" {
-		t.Errorf("Expected Use: 'custom [script]', got: '%s'", cmd.Use)
-	}
+	properties := []struct {
+		property string
+		expected string
+		actual   string
+	}{
+		{"Use", "custom [script]", cmd.Use},
+		{"Short", "Execute a custom query script", cmd.Short},
+		{"Long", "Execute a custom query script to perform tailored queries against the project's dependencies and dependents.", cmd.Long},
+	}
 
-	// ... similar blocks for other properties
+	for _, prop := range properties {
+		assert.Equal(t, prop.expected, prop.actual, 
+			"Expected %s: '%s', got: '%s'", prop.property, prop.expected, prop.actual)
+	}

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Check for storage initialization patterns in the codebase
# Look for storage initialization in parent commands or middleware

# Search for storage initialization patterns
rg -A 5 "graph\.Storage|storage\." cmd/

# Check for any middleware or initialization patterns
ast-grep --pattern 'func PreRun($$$) { $$ }'

#!/bin/bash
# Let's check for storage initialization patterns more thoroughly

# Check for storage initialization in the Run methods
rg -A 5 "func.*Run" cmd/ingest/

# Check for storage-related imports and usage
rg "\".*storage\"" cmd/ingest/

# Look for storage initialization in the parent command
cat cmd/ingest/ingest.go

#!/bin/bash
# Description: Verify that dependent packages have also removed storage parameter
# and there are no lingering references

# Check for any remaining storage parameter usage in dependent packages
echo "Checking for storage parameter in keys package..."
ast-grep --pattern 'func New(storage $_) $_' cmd/leaderboard/keys/

echo "Checking for storage parameter in custom package..."
ast-grep --pattern 'func New(storage $_) $_' cmd/leaderboard/custom/

# Check for any direct storage field references that might need updating
echo "Checking for storage field references..."
rg "storage\." cmd/leaderboard/

#!/bin/bash
# Let's try a different approach to verify the changes

# First, let's check the actual function signatures in these packages
echo "Checking function signatures in keys package..."
rg "^func New" cmd/leaderboard/keys/ -A 2

echo "Checking function signatures in custom package..."
rg "^func New" cmd/leaderboard/custom/ -A 2

# Let's also check for any storage-related imports or variables
echo "Checking for storage-related code in all leaderboard packages..."
rg "storage" cmd/leaderboard/ -A 1

#!/bin/bash
# Description: Check for any remaining storage parameter usage in New() functions
# across related packages to ensure consistent refactoring

# Search for New functions that still accept storage parameter
ast-grep --pattern 'func New(storage $_) $_'

# Search for any remaining direct storage parameter usage
rg -l "storage.*graph\.Storage"

#!/bin/bash
# Let's check the implementation of query.New() to understand how it handles storage now
ast-grep --pattern 'func New() *cobra.Command {
  $$$
}'

# Also check for any storage-related code in the query package
rg -l "storage" cmd/query/

# Check how other commands that removed storage parameter are handling it
ast-grep --pattern 'func New() *cobra.Command {
  $$$
}' cmd/ingest/ cmd/cache/ cmd/leaderboard/

#!/bin/bash
# Search for references to this command's New function
rg "sbom\.New\(" --type go

# Look for similar command registrations
rg "cobra\.Command.*sbom" --type go

#!/bin/bash
# Description: Check for existing test files and coverage
# Look for test files that might need updating
fd -e go -e _test.go scorecard cmd/ingest

# Check if there are any existing test cases for LoadDataFromPath
rg -A 5 "LoadDataFromPath" --type go --type test

#!/bin/bash
# Search for existing command initialization tests
rg -t go "func Test.*Command" -g "*test.go"

# Look for cobra command testing patterns
rg -t go "cobra.*testing" -g "*test.go"

#!/bin/bash
# Description: Verify that LoadDataFromPath is consistently called without storage parameter
# and that there are no remaining references to the old signature

# Check for any remaining calls with storage parameter
ast-grep --pattern 'LoadDataFromPath($storage, $_)'

# Check the function definition to confirm signature change
ast-grep --pattern 'func LoadDataFromPath($_) $_'

# Look for any comments or documentation that might need updating
rg -l "LoadDataFromPath.*storage" --type md

#!/bin/bash
# Let's search for all calls to LoadDataFromPath to ensure consistency
rg "LoadDataFromPath\(" -A 2 -B 2

# Also check for any potential interface definitions or related types
ast-grep --pattern 'type $_ interface {
  $$$
  LoadDataFromPath($$$)
  $$$
}'

# Check for any test files that might be using this function
fd test.go --exec grep -l "LoadDataFromPath"

#!/bin/bash
# Check for existing error handling tests
rg -l "TestRun.*Error" cmd/query/custom/

 if o.queryServiceClient == nil {
+	if !strings.HasPrefix(o.addr, "http://") && !strings.HasPrefix(o.addr, "https://") {
+		return fmt.Errorf("invalid address format: %s. Must start with http:// or https://", o.addr)
+	}
 	o.queryServiceClient = apiv1connect.NewQueryServiceClient(
 		http.DefaultClient,
 		o.addr,

#!/bin/bash
# Description: Check for any remaining instances of LoadDataFromPath with storage parameter
# and verify test coverage in related files

# Search for any remaining instances of LoadDataFromPath with storage parameter
rg -p "LoadDataFromPath\([^)]*storage[^)]*\)" --type go

# Find all test files that might be affected by this change
fd -e go -g "*_test.go" | while read -r file; do
  echo "=== $file ==="
  rg "LoadDataFromPath" "$file" -C 2
done

#!/bin/bash
# Let's examine the LoadDataFromPath function definition and its usage pattern
# to verify the consistency of the parameter changes

# Check the function definition
ast-grep --pattern 'func LoadDataFromPath($_) $$$'

# Look for the implementation and any recent changes
rg -p "func LoadDataFromPath" --type go -C 5

# Check for any imports of the ingest package to see usage patterns
rg "\".*ingest\"" --type go

#!/bin/bash
# Description: Verify all callers of LoadDataFromPath have been updated to remove the storage parameter

# Search for any remaining calls to LoadDataFromPath that might still pass storage
rg -p "LoadDataFromPath\([^)]*storage" 

# Find all current usages of LoadDataFromPath for manual verification
rg -p "LoadDataFromPath\(" -A 2

#!/bin/bash
# Description: Verify the existence of test data file and its format
# Test: Check if the SBOM test file exists and is valid JSON
if [ -f "testdata/osv-sboms/google_agi.sbom.json" ]; then
    jq empty "testdata/osv-sboms/google_agi.sbom.json" 2>/dev/null
    if [ $? -eq 0 ]; then
        echo "✓ Test data file exists and is valid JSON"
    else
        echo "✗ Test data file exists but is not valid JSON"
    fi
else
    echo "✗ Test data file not found"
fi